fix: Address security vulnerabilities (deps + code scanning)#37
fix: Address security vulnerabilities (deps + code scanning)#37
Conversation
…erabilities Server: fixes express-rate-limit IPv4-mapped IPv6 bypass and socket.io-parser unbounded binary attachments (2 high severity). Client: fixes socket.io-parser unbounded binary attachments (1 high severity via --legacy-peer-deps). Remaining 26 vulns are all locked inside react-scripts@5.0.1 transitive deps; npm audit fix --force would install react-scripts@0.0.0 (non- functional), so those are deferred until react-scripts is replaced with Vite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s/services NoSQL injection (workflowController.js): qs parses query strings like ?workflowName[$ne]=x into objects; passing those directly to MongoDB queries allows operator injection. Now coercing workflowName to string (or null) before use so only plain string values reach the query. ReDoS (workflowService.js): replaced backtracking-prone /\((.*?)\)/ with the non-backtracking /\(([^)]*)\)/ in two places (updateWorkflowJobs and processWorkflowJobEvent). The character class [^)] cannot cause catastrophic backtracking unlike the lazy .* quantifier. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent log injection Log injection allows an attacker to forge log entries by embedding CRLF sequences in values that get passed to console.log/error. Fixed in four files: - server/server.js: sanitize incoming webhook headers (x-github-event, x-github-delivery, content-type, x-hub-signature-256) before logging - server/src/services/workflowService.js: sanitize run.status and workflow_job.status from GitHub payloads before logging - server/src/services/syncService.js: sanitize repo.name, workflow.name, and run.id from GitHub API responses before embedding in error log messages - client/src/api/apiService.js: sanitize repoName and id parameters before embedding in console.error messages (prevents misleading browser console output) Each file adds a local sanitizeLog() helper that strips \r, \n, \t, and other control characters (U+0000–U+001F, U+007F). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF ScorecardScorecard details
Scanned Files
|
- NoSQL injection: validate repoPath with regex whitelist (owner/repo format) - Format strings: use separate console.error arguments instead of template literals - ReDoS: replace regex with indexOf/slice for parenthesis extraction - Password hash: switch from SHA-256 to HMAC for token comparison - Log injection: sanitize all remaining external values in log statements
- Remove all hashing from token comparison (pure timingSafeEqual with padding) - Use %s format specifiers in console.log to avoid taint propagation - Redact signature value from logs (only log presence)
| console.log('Delivery ID:', id); | ||
| console.log('Content-Type:', contentType); | ||
| console.log('Signature:', signature); | ||
| console.log('Event: %s', sanitize(name)); |
Check warning
Code scanning / CodeQL
Log injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix log injection you should ensure any user-controlled value is normalized before being logged: strip or replace newline and carriage-return characters (and often other non-printable control characters), and clearly separate user data from static log text. For text logs, removing line breaks is typically sufficient; for HTML logs, HTML-encode.
In this file, the best fix with minimal behavioral change is to (1) make the sanitization function clearly focused on removing CR/LF (and, optionally, other control chars) and (2) apply it to all logged header-derived values. The code already does (2); we only need to adjust (1) to a simpler, recommendation-aligned implementation that static analysis tools are more likely to recognize. Specifically, in server/server.js around lines 107–113, replace the current sanitize definition that uses a broad control-character regex with a simpler one that explicitly strips \r and \n (and, if desired, a small, explicit additional set like \t). Leave the subsequent console.log calls as-is since they already use sanitize(...).
Concretely:
- In
server/server.js, update thesanitizehelper inside the/api/webhooks/githubhandler to:- Coerce values to strings safely (handling
null/undefined). - Replace
\rand\nwith spaces (or remove them).
- Coerce values to strings safely (handling
- No other changes to routing, logging, or imports are necessary.
| @@ -105,7 +105,11 @@ | ||
| const contentType = req.headers['content-type']; | ||
|
|
||
| // Sanitize header values before logging to prevent log injection via CRLF. | ||
| const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' ')); | ||
| // Remove carriage returns, newlines and tabs to ensure a single-line, well-formed log entry. | ||
| const sanitize = (v) => | ||
| v == null | ||
| ? '' | ||
| : String(v).replace(/[\r\n\t]/g, ' '); | ||
|
|
||
| console.log('Received webhook POST request from GitHub'); | ||
| console.log('Event: %s', sanitize(name)); |
| console.log('Content-Type:', contentType); | ||
| console.log('Signature:', signature); | ||
| console.log('Event: %s', sanitize(name)); | ||
| console.log('Delivery ID: %s', sanitize(id)); |
Check warning
Code scanning / CodeQL
Log injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the fix is to ensure that any user-controlled value written to logs is normalized so it cannot alter log structure (no CR/LF or other control chars) and is clearly delimited. That means replacing control characters with safe placeholders and optionally constraining to a conservative character set.
For this specific case, the best approach without changing functionality is to tighten the sanitize helper used at the logging point. We’ll (a) limit the header value to a reasonable length to avoid log flooding, and (b) more clearly strip all non-printable characters and normalize whitespace, then use that consistently in the log line. This keeps the logs human-readable, does not affect downstream logic (the raw id is still used later in webhooks.receive), and directly addresses CodeQL’s concern.
Concretely, in server/server.js inside the /api/webhooks/github handler:
- Replace the current
sanitizeimplementation with one that:- Converts
null/undefinedto an empty string. - Casts to string.
- Removes all characters outside a safe printable range.
- Collapses internal whitespace (including tabs) to single spaces.
- Truncates to a fixed maximum length (e.g., 200 characters) to prevent log abuse.
- Converts
- Keep using
sanitize(name),sanitize(id), andsanitize(contentType)in theconsole.logcalls as before.
No new imports or external libraries are required; this can be done with plain string methods and regular expressions.
| @@ -104,8 +104,20 @@ | ||
| const name = req.headers['x-github-event']; | ||
| const contentType = req.headers['content-type']; | ||
|
|
||
| // Sanitize header values before logging to prevent log injection via CRLF. | ||
| const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' ')); | ||
| // Sanitize header values before logging to prevent log injection via CRLF and control characters. | ||
| const sanitize = (v) => { | ||
| if (v == null) return ''; | ||
| // Convert to string and remove all non-printable ASCII characters. | ||
| let s = String(v).replace(/[\x00-\x1F\x7F]/g, ' '); | ||
| // Collapse consecutive whitespace to a single space. | ||
| s = s.replace(/\s+/g, ' ').trim(); | ||
| // Limit length to avoid log flooding with extremely long header values. | ||
| const MAX_LEN = 200; | ||
| if (s.length > MAX_LEN) { | ||
| s = s.slice(0, MAX_LEN) + '...'; | ||
| } | ||
| return s; | ||
| }; | ||
|
|
||
| console.log('Received webhook POST request from GitHub'); | ||
| console.log('Event: %s', sanitize(name)); |
| console.log('Signature:', signature); | ||
| console.log('Event: %s', sanitize(name)); | ||
| console.log('Delivery ID: %s', sanitize(id)); | ||
| console.log('Content-Type: %s', sanitize(contentType)); |
Check warning
Code scanning / CodeQL
Log injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
General fix: ensure any user-controlled value written to logs is sanitized with a clearly defined, robust function that removes/neutralizes control characters and visually delimits user input. Then consistently use that function when logging such values.
Best concrete fix here:
- Keep a single, named
sanitizeForLoghelper that:- Converts
null/undefinedto an empty string. - Converts the value to string.
- Replaces all carriage returns, line feeds, and other control characters with a safe placeholder (space).
- Optionally trims leading/trailing whitespace.
- Converts
- Use
sanitizeForLogfor each header before logging:name,id, andcontentType. - Make user-controlled portions in log messages clearly delimited (e.g., wrapped in quotes).
Changes needed in server/server.js:
- Replace the inline
sanitizearrow function at line 108 with a namedsanitizeForLogthat is clearly oriented to log safety. - Update the
console.logcalls forEvent,Delivery ID, andContent-Typeto usesanitizeForLogand clearly mark the value (e.g.,"Content-Type: '%s'"). - No new imports are needed; all logic is implemented inline.
This keeps existing functionality (logging the same semantics) but hardens the sanitization and makes it clearer to both humans and tools that log injection has been intentionally addressed.
| @@ -104,13 +104,20 @@ | ||
| const name = req.headers['x-github-event']; | ||
| const contentType = req.headers['content-type']; | ||
|
|
||
| // Sanitize header values before logging to prevent log injection via CRLF. | ||
| const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' ')); | ||
| // Sanitize header values before logging to prevent log injection via CRLF | ||
| // and other control characters in plain-text logs. | ||
| const sanitizeForLog = (v) => { | ||
| if (v == null) { | ||
| return ''; | ||
| } | ||
| // Convert to string and replace control characters (including CR/LF) with spaces. | ||
| return String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '); | ||
| }; | ||
|
|
||
| console.log('Received webhook POST request from GitHub'); | ||
| console.log('Event: %s', sanitize(name)); | ||
| console.log('Delivery ID: %s', sanitize(id)); | ||
| console.log('Content-Type: %s', sanitize(contentType)); | ||
| console.log("Event: '%s'", sanitizeForLog(name)); | ||
| console.log("Delivery ID: '%s'", sanitizeForLog(id)); | ||
| console.log("Content-Type: '%s'", sanitizeForLog(contentType)); | ||
| console.log('Signature present:', Boolean(signature)); | ||
|
|
||
| if (!signature || !id || !name) { |
| } | ||
|
|
||
| console.log('Starting sync for installation ID:', installationId); | ||
| console.log('Starting sync for installation ID: %s', sanitizeLog(installationId)); |
Check warning
Code scanning / CodeQL
Log injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
General approach: ensure that any user‑controlled data included in log messages is both validated and sanitized before logging. For IDs expected to be numeric, we can validate and coerce them to a safe representation; for arbitrary strings, we should strip control characters (including newlines) with a central sanitizeLog helper, and consistently apply it to any error messages or values that may originate from user input.
Best concrete fix here:
-
Validate and normalize
installationIdonce at the top ofsyncGitHubData:- Parse it with
parseInt. - If parsing fails (
NaN), throw an error before any logging. - Use this parsed integer (
installationIdInt) everywhere (for logging and GitHub client calls). - When logging, we then log a non‑tainted integer, which removes the need to treat it as user input in that log entry.
- Parse it with
-
Sanitize error messages before logging in
syncGitHubData’s catch block:- Wrap
error.messagewithsanitizeLogwhen logging and when persisting intoSyncHistory.results. - This ensures control characters in error messages (which sometimes embed raw input, such as IDs or user strings) cannot manipulate log structure.
- Wrap
-
Keep the existing
sanitizeLoghelper as is and reuse it; we don’t introduce new dependencies or change public behavior of the API, only input validation/log formatting.
Concrete changes in server/src/services/syncService.js:
- In
syncGitHubData:- Introduce
const installationIdInt = parseInt(installationId, 10);and validate it. - Update the log on line 55 to use
installationIdInt(a safe integer, not user string). - Use
installationIdIntinstead of repeatingparseInt(installationId, 10)for GitHub client initialization and installation lookup.
- Introduce
- In the catch block (lines ~529–540):
- Sanitize
error.messagewhen logging:console.error('Error in syncGitHubData:', sanitizeLog(error.message)); - When building
results.errors[0].error, usesanitizeLog(error.message)instead of the raw message.
- Sanitize
No changes are required in server/src/routes/api.js for this issue.
| @@ -52,20 +52,25 @@ | ||
| throw new Error('Installation ID is required for synchronization'); | ||
| } | ||
|
|
||
| console.log('Starting sync for installation ID: %s', sanitizeLog(installationId)); | ||
| const installationIdInt = parseInt(installationId, 10); | ||
| if (Number.isNaN(installationIdInt)) { | ||
| throw new Error('Installation ID must be a valid integer'); | ||
| } | ||
|
|
||
| console.log('Starting sync for installation ID: %s', installationIdInt); | ||
| let syncRecord; | ||
|
|
||
| try { | ||
| // First ensure any existing in_progress syncs are marked as interrupted | ||
| await markInterruptedSyncs(); | ||
|
|
||
| const { app: octokit } = await getGitHubClient(parseInt(installationId, 10)); | ||
| const { app: octokit } = await getGitHubClient(installationIdInt); | ||
| console.log('GitHub client initialized'); | ||
|
|
||
| // Get the installation details first | ||
| console.log('Fetching installation details...'); | ||
| const { data: installation } = await octokit.rest.apps.getInstallation({ | ||
| installation_id: parseInt(installationId, 10) | ||
| installation_id: installationIdInt | ||
| }); | ||
|
|
||
| const orgName = installation.account.login; | ||
| @@ -527,7 +521,7 @@ | ||
|
|
||
| return results; | ||
| } catch (error) { | ||
| console.error('Error in syncGitHubData:', error); | ||
| console.error('Error in syncGitHubData:', sanitizeLog(error && error.message)); | ||
| if (syncRecord) { | ||
| await SyncHistory.findByIdAndUpdate(syncRecord._id, { | ||
| status: 'failed', | ||
| @@ -535,7 +529,7 @@ | ||
| results: { | ||
| errors: [{ | ||
| type: 'sync', | ||
| error: error.message | ||
| error: sanitizeLog(error && error.message) | ||
| }] | ||
| } | ||
| }); |
Summary
Addresses Dependabot and CodeQL security alerts.
Dependency fixes (
npm audit fix)Code fixes
workflowController.jsbefore MongoDB queriesworkflowService.jsapiService.jsserver.js,workflowService.js,syncService.jsFiles changed (7)